balance fetcher to fix non-eth assetIds#8284
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/assets-controller/src/data-sources/evm-rpc-services/services/BalanceFetcher.ts
Show resolved
Hide resolved
packages/assets-controller/src/data-sources/evm-rpc-services/services/BalanceFetcher.ts
Outdated
Show resolved
Hide resolved
packages/assets-controller/src/data-sources/evm-rpc-services/services/BalanceFetcher.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The goal here was to pass the assetId alongside the token addresses and the tokenInfo to the balance fetcher.
Since the last two were already being placed in two different arrays of the same length, as well as a setting to return native balance, it's all been simplified by just passing to balance fetcher a single array that contains everything needed:
- AssetId (including the native token)
- Address
- TokenInfo (this is just the decimals)
That way, BalanceFetcher does not need to know how to build the native assetId for every chain.
Also, the token address passed to balance fetcher is always the zero address, regardless of the chain, as that is how BalanceFetcher determines whether to fetch the balance of a token or the balance of the account.
packages/assets-controller/src/data-sources/evm-rpc-services/services/BalanceFetcher.test.ts
Outdated
Show resolved
Hide resolved
| * native assets even when the chain has a native asset with a non-zero address) | ||
| * and optional metadata | ||
| */ | ||
| export type BalanceFetchOptions = { |
There was a problem hiding this comment.
This wasn't being used at all, only to specify that we wanted to include native assets, which was already the default. In any case the native assets need to be added to the array of tokens to fetch now.
| /** Token decimals (omit when unknown — balance fetcher returns raw balance for RpcDataSource to resolve). */ | ||
| decimals?: number; | ||
| /** Token symbol (optional) */ | ||
| symbol?: string; |
There was a problem hiding this comment.
This wasn't being used at all.
| continue; | ||
| } | ||
|
|
||
| const isNative = assetNamespace === 'slip44'; |
There was a problem hiding this comment.
This is not completely right, but this is how we are currently determining that an asset is native everywhere else, so this is consistent.
We already have a ticket and plans to discuss how to handle native tokens (by pushing the definition to an API).
| const tokenMap = new Map<string, TokenFetchInfo>(); | ||
|
|
||
| for (const assetId of Object.keys(accountBalances)) { | ||
| // Only process ERC20 tokens on the current chain |
There was a problem hiding this comment.
We no longer need to filter only erc20 tokens, since we need to include all assets for that chain, including native assets.
This simplifies things as well.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
packages/assets-controller/src/data-sources/evm-rpc-services/services/BalanceFetcher.ts
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
bergarces
force-pushed
the
fix-rpc-balance-fetcher
branch
from
1189e6b to
4ab304a
Compare
bergarces
changed the title
|
|
||
| ### Fixed | ||
|
|
||
| - Refactored `BalanceFetcher` and `RpcDataSource` to ensure the correct `assetId` is used for EVM native assets that are not ETH ([#8284](https://github.com/MetaMask/core/pull/8284)) |
There was a problem hiding this comment.
All the method signatures I have changed are internal, none of them are used in the clients. So I'm not marking it as breaking.
|
|
||
| assetsToFetch.set(assetIdLowerCase, { | ||
| assetId, | ||
| address: tokenAddress, |
There was a problem hiding this comment.
Here we should also use the state that we fetch in order to get the decimals and pass them down, making it mandatory, not optional.
That stops us from having to guess the decimals afterwards.
I would prefer to do that in a separate PR though.
| */ | ||
| async _executePoll(input: BalancePollingInput): Promise<void> { | ||
| const result = await this.fetchBalances( | ||
| const result = await this.#fetchBalances( |
There was a problem hiding this comment.
we're converting this to internal function , this is good i like it
There was a problem hiding this comment.
Yes. I had a look at the public methods and a couple of them were only being used internally, so I proactively switched them to private for now to make it easier to see what's exposed.
2 participants
Explanation
Fixes issue in which the balance appears with the wrong assetId forslip44 evm native assets that are not ETH.
BalanceFetcher now requires the list of tokens to include assetId, address and the native token.
Before (basic functionality off, Polygon using RPC and the wrong assetId)
With basic functionality on, the Polygon balance uses the correct assetId (as it comes from the api), but Avalanche is still wrong.
After (basic functionality off):
Tested also with basic functionality on, which then has prices.
References
Checklist
Note
Medium Risk
Refactors the EVM RPC balance fetching pipeline and changes
BalanceFetcher’s public API, which could impact balance polling and custom asset balance retrieval across chains if any call sites weren’t updated correctly.Overview
Fixes incorrect
assetIdreporting for EVM native assets whose CAIP-19 identifier is noteip155:<chain>/slip44:60(e.g. Avalanche), by making callers pass the native asset ID through the RPC balance pipeline instead of havingBalanceFetcherreconstruct it.RpcDataSourcenow builds a unifiedAssetFetchEntry[](native + custom ERC-20s, with optional knowndecimals) and callsBalanceFetcher.fetchBalancesForAssets, which maps multicall results back to the originalassetId. The old token-address based API (fetchBalancesForTokens,TokenFetchInfo,BalanceFetchOptions) is removed, tests are updated accordingly, and a sharedZERO_ADDRESSconstant is introduced.Written by Cursor Bugbot for commit 2de1d56. This will update automatically on new commits. Configure here.